Skip to content

Conversation

MiaKoring
Copy link
Contributor

Is it already Christmas? No but I brought you a gift anyway.

Jokes aside.

This PR adds the .sheet Modifier, like we know it from SwiftUI with isPresented Binding and optional onDismiss callback.

Also some sheet modifiers:
presentationBackground
presentationDragIndicatorVisibility
presentationCornerRadius
presentationDetents
interactiveDismissDisabled

and Example usage in WindowingExample

support for the modifiers varies quite a bit. most are ignored if unsupported, interactiveDismissDisabled and presentationBackground are required as both shouldn’t be limited by Framework features.

Currently only UIKitBackend, AppKitBackend and GtkBackend support sheets, as I sadly couldn’t get them to work properly on WinUI. I hope someone else can build on the Foundation.

As was to expect, there are some weird quirks:
on UIKitBackend and AppKitBackend sheet content is placed top-leading. On GtkBackend on Linux Text elements are very small without further modification instead of allocation space. Same thing happened on macOS with GtkBackend in the 2nd and 3rd window (before and after my changes).

I expect those to be fixable through SCUI Frontend Code. If you know how to fix those in the Backends, I’d be happy to do so before a merge.

Copy link
Owner

@stackotter stackotter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love how much of the SwiftUI sheets API you've managed to implement in this PR! Sorry about all the requested changes; they're generally about the internals rather than the API surface. You've had to use a lot of SwiftCrossUI's more obscure internals like the preferences system and the Gtk bindings signal system, so it's completely understandable 😅 I'll test this out on my own devices locally when I get the time (probably tomorrow).

Comment on lines +809 to +811
public protocol SheetImplementation {
var sheetSize: SIMD2<Int> { get }
}
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using a protocol here is nice, but I've generally tried to keep the backend API really consistent in its design. Unfortunately we can't use protocols for a lot of the widget-related stuff so I've gone with the decision of making all backend data types opaque and putting their getters and setters on the backend. It's not super Swifty, but the goal is to make backend implementation surprise-free, so it's kinda the best we can do with today's Swift.

tl;dr remove the protocol and move sheetSize to a size(ofSheet:) method on the AppBackend protocol.

If there's a technical reason that it has to be done this way (e.g. to access sheet sizes in a place with no backend access or something), let me know

Comment on lines +607 to +608
/// Creates a sheet object (without showing it yet). Sheets contain View Content.
/// They optionally execute provied code on dismiss and
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make View Content lowercase (unless it links to a type).

Replace provied with provided.

/// prevent users from interacting with the parent window until dimissed.
func createSheet() -> Sheet

/// Updates the content and appearance of a sheet
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

End with a full stop (sorry, a bit pedantic cause it's for the public docs)

/// Updates the content and appearance of a sheet
func updateSheet(
_ sheet: Sheet,
content: Widget,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For views with a single child we generally take the widget in the creation method instead of the update method. Each view graph node is guaranteed to keep the same widget for its whole lifecycle so you can give the sheet creation method a reference the the sheet content's underlying widget before updating the widget with the sheets size (so there shouldn't be any circular dependencies).


/// Shows a sheet as a modal on top of or within the given window.
/// Users should be unable to interact with the parent window until the
/// sheet gets dismissed. The sheet will be closed once onDismiss gets called
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... `onDismiss` only gets called once the sheet has been closed.

var grid: WinUI.Grid
var cachedAppWindow: WinAppSDK.AppWindow!

//only for AppBackend conformance, no support yet
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// Only ...

Comment on lines +2 to +4
/// Presents a conditional modal overlay
/// onDismiss optional handler gets executed before
/// dismissing the sheet
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// Presents a conditional modal overlay. `onDismiss` gets invoked when the sheet is dismissed.

@ViewBuilder content: @escaping () -> SheetContent
) -> some View {
SheetModifier(
isPresented: isPresented, body: TupleView1(self), onDismiss: onDismiss,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrap each parameter onto its own line, and the closing ) onto its own as well (only cause it's wrapped);

foo(
    param1,
    param2,
    ...
)

Comment on lines +37 to +42
let sheetViewGraphNode = ViewGraphNode(
for: sheetContent(),
backend: backend,
environment: environment
)
let sheetContentNode = AnyViewGraphNode(sheetViewGraphNode)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Delay sheet content display until the sheet is presented (so that onAppear and onDisappear work correctly). You'll also have to set the sheet node to nil when the sheet is dismissed (so that the sheet node gets dropped and onDisappear gets triggered).

dryRun: true
)

let preferences = dryRunResult.preferences
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even though these should be the same as the final run, it's best to use the preferences from the final non-dryrun to avoid confusion if things go wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants